Feature: Add consistent logging errorprone rule #1644
Conversation
|
Depending on how controversial this is, we might consider auto-fixing this by adding here |
| summary = "Loggers created using getLogger(Class<?>) must be named 'log'.") | ||
| public final class ConsistentLoggerName extends BugChecker implements BugChecker.VariableTreeMatcher { | ||
|
|
||
| private static final Matcher<VariableTree> matcher = Matchers.allOf( |
There was a problem hiding this comment.
Thoughts on filtering down to static final fields in this case? I think we should add other rules to require loggers to be private+static+final, but in the case of odd legacy code we should flag the most specific failure. I'm not confident we'd want instance loggers to be renamed automatically, I just don't have a good enough sense for all the places they might exist.
|
+1 for auto-fixing this, please give the automatic fix a run on our large internal codebase to verify before we merge. |
| if (!matcher.matches(tree, state)) { | ||
| return Description.NO_MATCH; | ||
| if (matcher.matches(tree, state)) { | ||
| if (!tree.getName().contentEquals("log")) { |
There was a problem hiding this comment.
pedantic nit: might as well combine conditions to reduce indent depth if (matcher.matches(tree, state) && !tree.getName().contentEquals("log")) {
I'll give this PR a try internally
| private static final Matcher<VariableTree> matcher = Matchers.allOf( | ||
| Matchers.isField(), | ||
| Matchers.isStatic(), | ||
| MoreMatchers.isFinal(), |
There was a problem hiding this comment.
I think we can avoid adding a new utility:
| MoreMatchers.isFinal(), | |
| Matchers.hasModifier(Modifier.FINAL), |
That will match interface Iface { Logger log = LoggerFactory.getLogger(Iface.class); } as final which is why we had to implement the custom MoreMatchers.hasExplicitModifier for our RedundantModifier check.
| "Test.java", | ||
| "import org.slf4j.*;", | ||
| "class Test {", | ||
| " private static final Logger LOG = LoggerFactory.getLogger(Test.class);", |
There was a problem hiding this comment.
how about logger -> log? or do you think that's cool?
There was a problem hiding this comment.
Everything goes to log. There can only be 1 name for loggers
...ine-error-prone/src/test/java/com/palantir/baseline/errorprone/ConsistentLoggerNameTest.java
Outdated
Show resolved
Hide resolved
...ine-error-prone/src/test/java/com/palantir/baseline/errorprone/ConsistentLoggerNameTest.java
Outdated
Show resolved
Hide resolved
|
Released 3.68.0 |
Before this PR
Closes #1642
After this PR
==COMMIT_MSG==
Add consistent logging errorprone rule
==COMMIT_MSG==
Possible downsides?